Skip to content

Add defensive null checks and improve security in postMessage calls#376

Closed
bryancasler wants to merge 3 commits into
mainfrom
claude/code-audit-review-0g1nW
Closed

Add defensive null checks and improve security in postMessage calls#376
bryancasler wants to merge 3 commits into
mainfrom
claude/code-audit-review-0g1nW

Conversation

@bryancasler

Copy link
Copy Markdown
Member

Summary

This PR adds comprehensive null/undefined checks throughout the codebase to prevent runtime errors, improves security by replacing wildcard postMessage origins with specific domain origins, and refactors unsafe code evaluation to use a safe arithmetic parser.

Key Changes

Null Safety Improvements

  • DonationAmount: Added null checks before accessing DOM elements (otherField, otherWrapper) to prevent errors when elements don't exist
  • DonationFrequency: Wrapped sessionStorage access in try-catch blocks to handle cases where storage is unavailable or disabled
  • RememberMe: Added try-catch around JSON parsing to gracefully handle malformed cookie/postMessage data
  • ShowHideRadioCheckboxes: Fixed unsafe array mutation by replacing forEach with splice with a safer filter + push pattern

Security Enhancements

  • iFrame: Replaced wildcard "*" postMessage origins with this.parentOrigin derived from document.referrer to enforce same-origin policy
  • RememberMe: Added getRemoteOrigin() method to extract origin from remoteUrl instead of using wildcard, with fallback to "*" if parsing fails
  • UpsellLightbox: Replaced unsafe Function() constructor evaluation with a safe evaluateSuggestion() method that:
    • Validates expressions against a whitelist regex (/^[\d\s.+\-*/()]+$/)
    • Parses and evaluates simple binary arithmetic expressions safely
    • Returns 0 on invalid input or evaluation errors

Error Handling

  • DonationAmount: Wrapped setAmount() logic in try-finally to ensure _dispatch flag is always reset
  • DonationFrequency: Wrapped setRecurrency() and setFrequency() in try-finally blocks for consistent state management
  • App: Added retry counter (_runRetries) with a 50-retry limit to prevent infinite loops when Engaging Networks framework is unavailable

Implementation Details

  • All DOM queries now check for null before accessing properties
  • SessionStorage operations are wrapped in try-catch to handle private browsing and disabled storage scenarios
  • PostMessage security improved by deriving origins from URLs rather than using wildcards
  • Arithmetic evaluation uses a reduce-based parser instead of Function() constructor for better security

https://claude.ai/code/session_012mdWCSZqsc9D7iBiV4dur9

BUG-1: onLoad.bind() never invoked pre-existing handler (app.ts)
BUG-2/3: Null crash in DonationAmount.clearOther()/load() when "other" field missing
BUG-4: splice() during forEach skips consecutive elements (ShowHideRadioCheckboxes)
BUG-5: _dispatch flag not protected by try/finally in DonationAmount/DonationFrequency
BUG-6: Exit intent fires open() twice, duplicating analytics event
BUG-7: JSON.parse without try/catch in RememberMe.updateFieldData()
BUG-8: postMessage with wildcard "*" origin in RememberMe and iFrame
BUG-9: sessionStorage access not wrapped in try/catch in DonationFrequency
MOD-1: Replace Function() constructor with safe arithmetic evaluator in UpsellLightbox
MOD-3: Add retry limit (50) to EN framework detection loop in App.run()

https://claude.ai/code/session_012mdWCSZqsc9D7iBiV4dur9
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants